refactor: improve audio device initialization process #450
Conversation
- Enhance audio device selection with comprehensive testing and validation - Add device initialization and start tests before selecting a capture source - Improve error handling and logging for audio device detection - Update capture function to pass selected device source directly - Add platform-specific backend selection for malgo context
…nitor lifecycle - Modify buffer initialization to collect and report multiple errors - Update control monitor to use a dedicated done channel for cleaner synchronization - Enhance error logging for buffer initialization with more informative messages - Prevent premature function exit when buffer initialization partially fails
WalkthroughThe changes enhance the functionality of audio analysis and capture. In Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Application
participant Analysis as RealtimeAnalysis
participant Buffer as BufferManager
participant Monitor as ControlMonitor
Main->>Analysis: Initialize real-time analysis
Analysis->>Buffer: Call initializeBuffers(sources)
Buffer-->>Analysis: Return aggregated errors (if any)
Analysis->>Monitor: Start control monitor
Monitor-->>Analysis: Signal monitor completion
sequenceDiagram
participant App as Application
participant Capture as CaptureAudio
participant Selector as selectCaptureSource
participant Device as malgo Device
App->>Capture: Invoke CaptureAudio with settings
Capture->>Selector: Request capture source
Selector->>Device: Initialize and test audio device
alt Device initialization succeeds
Device-->>Selector: Return captureSource
Selector-->>Capture: Return captureSource
Capture->>Device: Call captureAudioMalgo(captureSource)
Device-->>Capture: Audio capture started
else Device initialization fails
Device-->>Selector: Log error and fallback
Selector-->>Capture: Return error, try alternate source
end
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
- Remove unnecessary monitorDone channel in startControlMonitor - Streamline goroutine exit mechanism - Remove redundant channel closing and waiting logic
- Introduce initializeBuffersForSource to centralize buffer setup logic - Improve error handling and cleanup for RTSP and local audio sources - Reduce code duplication in CaptureAudio function - Enhance error logging for buffer initialization failures
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/myaudio/capture.go (2)
364-364: Consider handling errors from deferred cleanup.While the comment suggests errors are handled in the caller, the error from
malgoCtx.Uninit()is actually not propagated. Consider wrapping the cleanup in a named return error:-defer malgoCtx.Uninit() //nolint:errcheck // We handle errors in the caller +var retErr error +defer func() { + if cleanupErr := malgoCtx.Uninit(); cleanupErr != nil && retErr == nil { + retErr = fmt.Errorf("cleanup error: %w", cleanupErr) + } +}()
491-491: Handle errors in deferred cleanup calls.Similar to the previous comment, consider handling errors from deferred cleanup calls:
malgoCtx.Uninit()on line 491captureDevice.Stop()on line 594-defer malgoCtx.Uninit() //nolint:errcheck // We handle errors in the caller +defer func() { + if err := malgoCtx.Uninit(); err != nil { + log.Printf("❌ Error during context cleanup: %v", err) + } +}() -defer captureDevice.Stop() //nolint:errcheck // We handle errors in the caller +defer func() { + if err := captureDevice.Stop(); err != nil { + log.Printf("❌ Error stopping capture device: %v", err) + } +}()Also applies to: 594-594
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/myaudio/capture.go(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test-docker-image
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
internal/myaudio/capture.go (3)
263-298: LGTM! Well-structured buffer initialization.The function is thread-safe, handles errors properly, and includes cleanup on failure. The implementation follows good practices.
300-340: LGTM! Improved error handling and user experience.The changes successfully implement the PR objective by allowing the application to continue without a functioning capture device, while providing clear feedback to users.
559-564: LGTM! Well-implemented restart logic.The restart logic properly handles both the restart signal and application shutdown with appropriate channel operations and timeouts.
|
@tphakala the container seems to start now with this change, but my RTSP source doesn't seem to be actually working, (I dont see the indicator showing sound level on the UI) |
|
Are you sure your RTSP source isn't working? According log you provided ffmpeg was started
Audio level indicator is now a menu item, you can choose which audio source level is displayed. I just realised that it seems to default to sysdefault (sound card source) even though there isn't sound card source available, if you click level indicator and choose RTSP url you should see some activity. |
You are certainly right, I didn't know that, I see it working now on the latest dev! Do you want me to open an issue to cache the last selected source or some other logic to not-default-to-an-undesired-default-one for a setup like mine? I know I was jumping around a bit from place to place but I just wanted to help a bit with an alternate setup as you started to gear up for the next stable release |
Allow starting application without any working capture device